Mul, Sub and Add Layer with broadcasting support#186
Conversation
Semyon1104
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR implements a BinaryOpLayer class that supports element-wise multiplication, addition, and subtraction operations with broadcasting support for tensors. It extends existing layer functionality to handle binary operations between tensors of different but compatible shapes.
Key changes:
- New BinaryOpLayer class with support for multiplication, addition, and subtraction operations
- Broadcasting logic to handle tensors with compatible but different shapes
- Scalar tensor operations for element-wise operations with single values
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
include/layers/BinaryOpLayer.hpp |
Header file defining the BinaryOpLayer class interface and operation enums |
src/layers/BinaryOpLayer.cpp |
Implementation of binary operations with broadcasting and scalar support |
include/layers/Shape.hpp |
Added equality operators for Shape class to support testing |
test/single_layer/test_binaryoplayer.cpp |
Comprehensive test suite covering various broadcasting scenarios and operations |
Comments suppressed due to low confidence (2)
test/single_layer/test_binaryoplayer.cpp:180
- Test is named 'BroadcastingTestAdd' but is part of the 'BinaryOpLayerMulTests' test fixture. Consider creating a separate test fixture for Add operations or renaming to reflect the actual operation being tested.
TEST_F(BinaryOpLayerMulTests, BroadcastingTestAdd) {
test/single_layer/test_binaryoplayer.cpp:204
- Test is named 'BroadcastingTestSubGooglNet' but is part of the 'BinaryOpLayerMulTests' test fixture. Consider creating a separate test fixture for Sub operations or renaming to reflect the actual operation being tested.
TEST_F(BinaryOpLayerMulTests, BroadcastingTestSubGooglNet) {
|
|
||
| namespace itlab_2023 { | ||
|
|
||
| class BinaryOpLayer : public Layer { |
There was a problem hiding this comment.
It's part of elementwise operation
There was a problem hiding this comment.
I thought it would be better to put the logic of interaction between two tensors with a fairly comprehensive broadcast logic in an another layer to separate the situations where the input is a single tensor, EWLayer, and where there are two input arrays and 1 output
There was a problem hiding this comment.
Or is it better to combine these layers into one?
There was a problem hiding this comment.
Ok we can choose compromise, please create operation - Logic and unite all logic operations
There was a problem hiding this comment.
Do you mean to add the kDiv operation and name the enumerated type logic?
|
The current implementation recalculates indices for each element. Consider caching stride calculations for better performance with large tensors. |
|
While Mul, Add, and Sub are implemented, division would complete the basic arithmetic set. Consider adding kDiv to the BinaryOpType enum. |
I will also add the functionality of element-by-element subtraction and element-by-element addition layers - sub and add